Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stdlib] Rename py_object to py_object_ptr #3605

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from
Open

[stdlib] Rename py_object to py_object_ptr #3605

wants to merge 1 commit into from

Conversation

jjvraw
Copy link
Contributor

@jjvraw jjvraw commented Oct 4, 2024 •
edited
Loading

Apologies if this needs discussion. However, while working in these files, It seems odd to have an attribute of PythonObject named python_object that stores a PyObjectPtr.

struct PythonObject(...):
    var py_object: PyObjectPtr # rename py_object_ptr
    
...
 
     fn unsafe_as_py_object_ptr(self) -> PyObjectPtr:
        """Get the underlying PyObject pointer.

        Returns:
            The underlying PyObject pointer.

        Safety:
            Use-after-free: The caller must take care that `self` outlives the
            usage of the pointer returned by this function.
        """
        return self.py_object

This will cause conflicts with

@jjvraw jjvraw requested a review from a team as a code owner October 4, 2024 16:59
@jjvraw
Copy link
Contributor Author

jjvraw commented Oct 4, 2024

Also, might be beneficial to rename unsafe_as_py_object_ptr to unsafe_py_object_ptr for consistency. Happy to add that in here as well.

@JoeLoser JoeLoser requested a review from ConnorGray October 4, 2024 17:24
@JoeLoser
Copy link
Collaborator

JoeLoser commented Oct 4, 2024

@ConnorGray curious to get your thoughts and eyes here.

@ConnorGray
Copy link
Collaborator

Also, might be beneficial to rename unsafe_as_py_object_ptr to unsafe_py_object_ptr for consistency. Happy to add that in here as well.

+1 to this, that function isn't used widely so happy to see that small cleanup here as well :)

@JoeLoser
Copy link
Collaborator

I'll defer to @ConnorGray on the final call here, but I'll sync this in regardless. Thanks!

@JoeLoser
Copy link
Collaborator

!sync

@JoeLoser JoeLoser assigned ConnorGray and unassigned JoeLoser Oct 17, 2024
@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Oct 17, 2024
@jjvraw
squash and merge
669682f 
Signed-off-by: Joshua James Venter <[email protected]>
@jjvraw
Copy link
Contributor Author

jjvraw commented Nov 19, 2024

Ola :) Gentle reminder @ConnorGray, if this is still wanted.

Apologies for the delayed rebase. End of uni semester had my attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ConnorGray ConnorGray Awaiting requested review from ConnorGray

At least 1 approving review is required to merge this pull request.

Assignees

@ConnorGray ConnorGray

Labels
imported-internally Signals that a given pull request has been imported internally.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jjvraw @JoeLoser @ConnorGray @modularbot